Skip to content

Conversation

@johnzhou721
Copy link
Contributor

@johnzhou721 johnzhou721 commented Nov 27, 2025

Refs #3069.

Adds initial support for Window, Application, and ActivityIndicator for libadwaita. This PR thus makes all currently implemented GTK4 functionalities compatible with libadwaita.

Corners are now rounded -- yay!!!

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@johnzhou721 johnzhou721 marked this pull request as draft November 27, 2025 04:44
Copy link
Contributor Author

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments inline about the way things are in this PR.

This PR makes all currently implemented GTK4 functionalities compatible with libadwaita, which is why ActivityIndicator is in here.

Ready for review.

Comment on lines +155 to +159
"cover-if-plain-gtk4": "os_environ.get('TOGA_GTK', '') != '4' or "
"os_environ.get('TOGA_GTKLIB', '') != ''",
"cover-if-plain-gtk": "os_environ.get('TOGA_GTKLIB', '') != ''",
"cover-if-libadwaita": "os_environ.get('TOGA_GTK', '') != '4' or "
"os_environ.get('TOGA_GTKLIB', '') != 'Adw'",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is following the no-cover prefix convention perhaps better here? I thought it'd be better to be like "cover [only] if plain gtk" rather than the contrapositive "no cover if not plain gtk".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "no cover" prefix has historical precedence in testing, but the "if" part doesn't have to be preserved - so no-cover-unless-adwaita would be viable

if Adw is None:
assert isinstance(self.app._impl.native, Gtk.Application)
else:
assert isinstance(self.app._impl.native, Adw.Application)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, Adw.Application subclasses Gtk.Application... however, I used this stronger assertion because without Adw.Application, window bottom corners doesn't round at all.

Same with Adw.Window

Comment on lines +6 to +7
# libadwaita 1.6.0 is not in Ubuntu 24.04 yet; no-cover it.
if Adw is not None and ADW_VERSION >= (1, 6, 0): # pragma: no cover
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this no-cover perhaps makes you nervous? Should we land Adwaita 1.6.0 changes until all majors distros ship it? Right now Ubuntu 24.04 does not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the basis that this means we're not actually testing half the implementation - yes, this does make me nervous.

I'd need more context to make a call on how to handle this though. What's the motivation for setting a 1.6 minimum? What distributions do provide 1.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freakboy3742 Adw.Spinner was added in libadwaita 1.6 only.

Debian 13 has libadwaita 1.8, even Fedora 41 has libadwaita 1.6.0, Arch has 1.8.2, OpenSUSE Tumbleweed has 1.8.2.

It seems that Ubuntu 24.04 is the only one that does not provide libadwaita 1.6.

Ubuntu 24.04 also doesn't provide python3-pyside6.MODULE pcakages... I wonder what the heck happened with it. I just can't wait until next August...

Comment on lines 34 to 38
GLIB_VERSION: tuple[int, int, int] = (
GLib.MAJOR_VERSION,
GLib.MAJOR_VERSION,
GLib.MAJOR_VERSION,
GLib.MINOR_VERSION,
GLib.MICRO_VERSION,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe I didn't catch this before...

Comment on lines +225 to +227
- "linux-wayland-gtk4-adw"
- "linux-wayland-qt"
- "linux-x11-qt"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI -- the Qt things worked before because Qt overrode all the variables in the include section.

$ export TOGA_GTK=4
```

The experimental GTK 4 backend also aims to provides support for integrating with desktop environment-specific libraries. At present, `libadwaita` is the only supported library of this kind. This functionality requires libadwaita 1.5 or newer. To enable libadwaita integration support, set:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libadwaita 1.5 changed the way dialog things are handled, and also the application "about" dialogs. The relevant APIs are deprecated in libadwaita 1.6.

All major distros ship libadwaita 1.5+. Most major distros ship libadwaita 1.6+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to make libadwaita support optional/opt in? Based on your previous descriptions, it's the sort of thing that, if available, should be Just Used - or, at the very least, opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freakboy3742 But there's non-GNOME GTK distros. Should we say that they're minor enough to assume GNOME by default?

Or we could also check XDG_CURRENT_DESKTOP as a default. Which option would you prefer?

@johnzhou721 johnzhou721 marked this pull request as ready for review November 28, 2025 02:14
@johnzhou721

This comment was marked as resolved.

@kattni
Copy link
Contributor

kattni commented Dec 7, 2025

@johnzhou721 Can you verify whether this is ready for review? Thanks.

@johnzhou721
Copy link
Contributor Author

@kattni Verified. I explicitedly stated this and resolved the next comment about CI fails.

@kattni
Copy link
Contributor

kattni commented Dec 8, 2025

@johnzhou721 When something is ready for review, that means that there are no other changes necessary. When there are commits after you state that it is ready for review, the assumption will be that it is not, in fact, ready for review. Therefore, I will verify with you whether it is actually ready, before handing it off.

In the future, if you would like to avoid this, please ensure that you have all your intended changes completed before asking for a review, and if you do need to submit more changes, please restate that it is ready once you have completed those.

Remove the 'invalid_size_while_hidden' attribute from BoxProbe.
@johnzhou721
Copy link
Contributor Author

@kattni Thanks for letting me know about what caused the confusion. I'll try to avoid doing this in the future, but in this case it was neccesary.

For a specific explanation, the commit resolves an artifact from the merge; it was hard for me to test all the environments at once locally. However I understand that this is not an excuse — and I apologize for spending a bit of your time doing the verification.

Since then I realized an error in the commit you referenced -- I've pushed another update, and reconfirming this is ready for review.

@kattni kattni requested a review from freakboy3742 December 8, 2025 01:45
@kattni
Copy link
Contributor

kattni commented Dec 8, 2025

@johnzhou721 Thank you for following up. I've handed this off to Russell.

To be clear, I'm not saying that you can't submit subsequent commits, I'm simply saying that you need to let us know afterwards that it is again ready for review.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/comments about testing and the opt-in nature of the proposed behavior.

$ export TOGA_GTK=4
```

The experimental GTK 4 backend also aims to provides support for integrating with desktop environment-specific libraries. At present, `libadwaita` is the only supported library of this kind. This functionality requires libadwaita 1.5 or newer. To enable libadwaita integration support, set:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to make libadwaita support optional/opt in? Based on your previous descriptions, it's the sort of thing that, if available, should be Just Used - or, at the very least, opt-out.

Comment on lines +6 to +7
# libadwaita 1.6.0 is not in Ubuntu 24.04 yet; no-cover it.
if Adw is not None and ADW_VERSION >= (1, 6, 0): # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the basis that this means we're not actually testing half the implementation - yes, this does make me nervous.

I'd need more context to make a call on how to handle this though. What's the motivation for setting a 1.6 minimum? What distributions do provide 1.6?

Comment on lines +155 to +159
"cover-if-plain-gtk4": "os_environ.get('TOGA_GTK', '') != '4' or "
"os_environ.get('TOGA_GTKLIB', '') != ''",
"cover-if-plain-gtk": "os_environ.get('TOGA_GTKLIB', '') != ''",
"cover-if-libadwaita": "os_environ.get('TOGA_GTK', '') != '4' or "
"os_environ.get('TOGA_GTKLIB', '') != 'Adw'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "no cover" prefix has historical precedence in testing, but the "if" part doesn't have to be preserved - so no-cover-unless-adwaita would be viable

johnzhou721 and others added 3 commits December 12, 2025 16:50
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Added possessive forms for 'elementaryOS' and 'tauOS'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants